Added cross-browser compatibility for gekko and blink based browsers#123
Added cross-browser compatibility for gekko and blink based browsers#123Saksham-Sirohi wants to merge 5 commits intofossasia:masterfrom
Conversation
Reviewer's GuideThis PR standardizes browser API usage by injecting a polyfill for unified chrome/browser support, migrating all storage calls to browser.storage, updating the manifest for Firefox add-on installation, and adding a Firefox-specific date-input fallback in the popup. Sequence Diagram: Unified Storage API CallssequenceDiagram
participant ES as Extension Script (e.g., popup.js)
participant UBA as Unified Browser API (browser.*)
ES->>UBA: browser.storage.local.get("someKey")
activate UBA
UBA-->>ES: Returns stored value / Promise
deactivate UBA
ES->>UBA: browser.storage.local.set({key: "value"})
activate UBA
UBA-->>ES: Callback / Promise on completion
deactivate UBA
Class Diagram:
|
| Change | Details | Files |
|---|---|---|
| Unified browser API with a polyfill injection |
|
src/scripts/browser-polyfill.jssrc/manifest.jsonsrc/popup.html |
| Migrated storage API calls from chrome.storage to browser.storage |
|
src/scripts/main.jssrc/scripts/popup.jssrc/scripts/scrumHelper.js |
| Updated manifest.json for Firefox add-on support |
|
src/manifest.json |
| Implemented Firefox date-input fallback |
|
src/scripts/popup.js |
Assessment against linked issues
| Issue | Objective | Addressed | Explanation |
|---|---|---|---|
| #120 | Extend the extension's compatibility to Gecko-based browsers (e.g., Firefox). | ✅ | |
| #120 | Maintain compatibility with currently supported browsers. | ✅ |
Possibly linked issues
- Extend browser compatibility to Gecko-based browsers while maintaining current compatibility #120: The PR adds cross-browser compatibility for Firefox by using a polyfill and updating the manifest, directly addressing the issue.
- Extend browser compatibility to Gecko-based browsers while maintaining current compatibility #120: The PR adds cross-browser compatibility for Firefox and includes a Firefox-specific date-input fallback in the popup to fix the UI issue with dates.
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai reviewon the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issueto create an issue from it. - Generate a pull request title: Write
@sourcery-aianywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai titleon the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summaryanywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summaryon the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guideon the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolveon the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismisson the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai reviewto trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Undefined isFirefox in popup.js (link)
General comments:
- You’re using
isFirefoxin popup.js without declaring it there—add a Firefox detection (e.g.const isFirefox = /Firefox/.test(navigator.userAgent)) at the top of popup.js so the date‐input workaround actually runs. - The change to manifest.json removes the Chrome
background.service_workersection—ensure you conditionally include or maintain the service worker entry so Chrome extensions still load the background script correctly. - Consider extracting repeated
browser.storage.local.get/setcalls into a small helper so you don’t have to manually swapchrome/browserin every method and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re using `isFirefox` in popup.js without declaring it there—add a Firefox detection (e.g. `const isFirefox = /Firefox/.test(navigator.userAgent)`) at the top of popup.js so the date‐input workaround actually runs.
- The change to manifest.json removes the Chrome `background.service_worker` section—ensure you conditionally include or maintain the service worker entry so Chrome extensions still load the background script correctly.
- Consider extracting repeated `browser.storage.local.get/set` calls into a small helper so you don’t have to manually swap `chrome`/`browser` in every method and reduce duplication.
## Individual Comments
### Comment 1
<location> `src/scripts/popup.js:47` </location>
<code_context>
document.addEventListener('DOMContentLoaded', function() {
+ // Firefox date input fix
+ if (isFirefox) {
+ document.querySelectorAll('input[type="date"]').forEach(input => {
+ if (!input.value) {
</code_context>
<issue_to_address>
Undefined isFirefox in popup.js
`isFirefox` must be defined or imported before use to avoid a ReferenceError.
</issue_to_address>
### Comment 2
<location> `src/scripts/browser-polyfill.js:2` </location>
<code_context>
+// scripts/browser-polyfill.js
+if (typeof browser === "undefined") {
+ window.browser = typeof chrome !== "undefined" ? chrome : {};
+}
</code_context>
<issue_to_address>
Polyfill doesn't override native browser API in Firefox
Firefox's native `browser` object uses promises, but your code expects callbacks. To ensure consistent behavior across browsers, use Mozilla's webextension-polyfill.
Suggested implementation:
```javascript
import browser from 'webextension-polyfill';
window.browser = browser;
```
1. Make sure to install the `webextension-polyfill` package in your project:
```
npm install webextension-polyfill
```
2. If your build system does not support ES modules or import syntax in this context, you may need to use a CDN or require statement, or adjust your bundler configuration.
3. Ensure this polyfill is loaded before any other scripts that use the `browser` API.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes and found some issues that need to be addressed.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
6d8056f to
7110418
Compare
7110418 to
1d5f638
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @Saksham-Sirohi - I've reviewed your changes - here's some feedback:
- Extract the repeated browser.storage.local.get/set logic into a small helper module to reduce duplication and simplify maintenance.
- Rename
getbrowserDatatogetBrowserData(or another consistent camelCase) to match the rest of your function naming conventions. - Make sure
browser-polyfill.jsis loaded before any other scripts (in both popup.html and content scripts) so that thebrowsernamespace is always available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the repeated browser.storage.local.get/set logic into a small helper module to reduce duplication and simplify maintenance.
- Rename `getbrowserData` to `getBrowserData` (or another consistent camelCase) to match the rest of your function naming conventions.
- Make sure `browser-polyfill.js` is loaded before any other scripts (in both popup.html and content scripts) so that the `browser` namespace is always available.
## Individual Comments
### Comment 1
<location> `src/scripts/browser-polyfill.js:2` </location>
<code_context>
+// scripts/browser-polyfill.js
+if (typeof browser === "undefined") {
+ window.browser = typeof chrome !== "undefined" ? chrome : {};
+}
</code_context>
<issue_to_address>
Polyfill only aliases browser to chrome without promise conversion
This approach does not provide promise-based API support. Using `webextension-polyfill` would ensure consistent promise support across browsers.
Suggested implementation:
```javascript
/**
* scripts/browser-polyfill.js
* Use the official webextension-polyfill to provide a consistent, promise-based browser API.
* See: https://github.com/mozilla/webextension-polyfill
*/
// Import the polyfill if not already present
// You must install webextension-polyfill as a dependency: npm install webextension-polyfill
import browser from 'webextension-polyfill';
if (typeof window !== "undefined") {
window.browser = browser;
}
```
- Ensure that `webextension-polyfill` is installed in your project (`npm install webextension-polyfill`).
- If your build system does not support ES modules or import syntax in this context, you may need to use a different approach to include the polyfill, such as referencing the CDN version or using a bundler.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@Saksham-Sirohi I have tested these changes in the Firefox browser, but the scrum report is not inserted on platforms like Outlook and Yahoo. Check that once. Thanks! |
|
@hpdang, I will open a separate issue for Yahoo and Outlook issue for someone to take |
Fixes #120
Summary by Sourcery
Enable cross-browser support by replacing Chrome-only APIs with the standardized browser namespace, adding a compatibility polyfill, and updating manifest settings for Firefox.
New Features:
Enhancements: